Disable main_thread_id assertion for Android debug build by jeyum2 · Pull Request #780 · godot-rust/gdext

jeyum2 · 2024-06-26T09:27:32Z

This assertion block was previously commented out and reactivated without a clear reason in git history
It causes a runtime panic in debug builds for Android
If this assertion is required, please implement logic that also considers Android

Bromeon · 2024-06-26T09:32:48Z

Thank you! Could you add a comment re. why it's commented-out? To prevent this from happening again.

Please then squash commits to 1 🙂

jeyum2 · 2024-06-26T09:44:28Z

The assertion was revived on 82a14f68

I’m not sure about the current status of Linux hot-reloading.
If it works on Linux, I will just add a comment for Android.
If not, I will restore the original comment with an additional comment for Android.

Bromeon · 2024-06-26T11:32:02Z

What about expanding the if cfg!(...) condition to include a Linux or Android test?

Followed by an explanation why on Android this doesn't work, that would be really valuable.

jeyum2 · 2024-06-26T12:41:05Z

I have added checking Android to the existing cfg condition
In this PR, we focus only on Android and suggest not modifying the previous code

GodotRust · 2024-06-27T00:29:14Z

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-780

Bromeon

There's a problem with this change, which is explained in the assert message:

attempted to access binding from different thread than main thread; this is UB

The whole rest of the library relies on access being single-threaded -- if we just disable the check, we might really run into UB.

Does anyone familiar with Android know why access happens from a different thread in Android? And if that's fine, or what the best practices are to deal with this?

Maybe one of the people involved in #470, e.g. @scripturial @moberer @MeganSpencer @Jim-Bar @walksanatora?

Bromeon · 2024-07-08T18:07:39Z

godot-ffi/src/binding/single_threaded.rs

+        // CHECK Need to check on more platforms.
+        // Currently, a runtime panic occurs on Android.
+        if cfg!(all(debug_assertions, not(target_os = "android"))) {


You probably mean TODO: instead of CHECK?

scripturial · 2024-07-09T04:50:44Z

The whole rest of the library relies on access being single-threaded -- if we just disable the check, we might really run into UB.

Does anyone familiar with Android know why access happens from a different thread in Android? And if that's fine, or what the best practices are to deal with this?

I don’t feel qualified to comment on Godot threading issues but I do have a suspicion regarding the cause of the problem I had. I didn’t realise callbacks from http requests in Godot could be on a separate thread. (I assume this is the case.) so when crossing into rust it’s possible that you end up in rust on a different thread (without realising). I think this can then cause updates to data or callbacks into Godot to occasionally crash.

The work around for me has to call back from rust into Godot using ‘call_deferred’ I’m not sure if it is the design of call_deferred to force “cross to main thread” but since I started doing this I’ve not had any android crashes.

The Swift plugin developer that made one of my plugins does the same “hack” where you call back into a gdscript function then invoke call_deferred which makes it rather messy.

It may be that this patch actually would have told me what I did wrong straight away and helped me solve the problem quicker. So maybe it’s a win?

(On hot reloading specifically, I use Mac for development so I can’t comment on hot reloading specifically)

Bromeon · 2024-07-09T07:19:58Z

@scripturial Thanks for the info!

The Swift plugin developer that made one of my plugins does the same “hack” where you call back into a gdscript function then invoke call_deferred which makes it rather messy.

Could you link me to that plugin and/or issue they had?

I don’t feel qualified to comment on Godot threading issues but I do have a suspicion regarding the cause of the problem I had. I didn’t realise callbacks from http requests in Godot could be on a separate thread. (I assume this is the case.) so when crossing into rust it’s possible that you end up in rust on a different thread (without realising). I think this can then cause updates to data or callbacks into Godot to occasionally crash.

Do you have any suspicion why this might happen on Android but not on other platforms?

Or maybe @jeyum2, can you be more specific about the circumstances which caused panic in your case? Can you come up with a minimal, complete example that reproduces the problem?

Bromeon · 2024-07-14T07:37:20Z

I've just merged #794 which addressed a similar issue, with Wasm. Could you rebase the PR onto latest master and resolve the conflicts?

Also, would be great to hear about the panic circumstances 🙂

Or maybe @jeyum2, can you be more specific about the circumstances which caused panic in your case? Can you come up with a minimal, complete example that reproduces the problem?

Thank you!

jeyum2 · 2024-07-14T22:23:48Z

@scripturial Thanks for the info!

The Swift plugin developer that made one of my plugins does the same “hack” where you call back into a gdscript function then invoke call_deferred which makes it rather messy.

Could you link me to that plugin and/or issue they had?

I don’t feel qualified to comment on Godot threading issues but I do have a suspicion regarding the cause of the problem I had. I didn’t realise callbacks from http requests in Godot could be on a separate thread. (I assume this is the case.) so when crossing into rust it’s possible that you end up in rust on a different thread (without realising). I think this can then cause updates to data or callbacks into Godot to occasionally crash.

Do you have any suspicion why this might happen on Android but not on other platforms?

Or maybe @jeyum2, can you be more specific about the circumstances which caused panic in your case? Can you come up with a minimal, complete example that reproduces the problem?

When running a debug build of an empty project, a panic occurs immediately during the initialization process.
This is the same issue as discussed in this Discord thread

03-17 18:19:18.394 16927 16927 F DEBUG   :       #01 pc 0000000000624564  /data/app/~~Ks3JY4pqes9m0gwzSWYDQw==/org.godotengine.tarnish-7ALG45aOnZwqwsvmqkQjBQ==/lib/arm64/libtarnish_rust.so (std::sys::unix::abort_internal::h98c5b5a7186aa89f+4)
03-17 18:19:18.394 16927 16927 F DEBUG   :       #02 pc 0000000000622734  /data/app/~~Ks3JY4pqes9m0gwzSWYDQw==/org.godotengine.tarnish-7ALG45aOnZwqwsvmqkQjBQ==/lib/arm64/libtarnish_rust.so (rust_panic+88)
03-17 18:19:18.394 16927 16927 F DEBUG   :       #03 pc 0000000000622508  /data/app/~~Ks3JY4pqes9m0gwzSWYDQw==/org.godotengine.tarnish-7ALG45aOnZwqwsvmqkQjBQ==/lib/arm64/libtarnish_rust.so (std::panicking::rust_panic_with_hook::hda05105b262e03ce+664)
03-17 18:19:18.394 16927 16927 F DEBUG   :       #04 pc 0000000000622238  /data/app/~~Ks3JY4pqes9m0gwzSWYDQw==/org.godotengine.tarnish-7ALG45aOnZwqwsvmqkQjBQ==/lib/arm64/libtarnish_rust.so (std::panicking::begin_panic_handler::_$u7b$$u7b$closure$u7d$$u7d$::h5062df77b9a38169+148)
03-17 18:19:18.394 16927 16927 F DEBUG   :       #05 pc 0000000000621004  /data/app/~~Ks3JY4pqes9m0gwzSWYDQw==/org.godotengine.tarnish-7ALG45aOnZwqwsvmqkQjBQ==/lib/arm64/libtarnish_rust.so (std::sys_common::backtrace::__rust_end_short_backtrace::h1cde41c1fc685d95+4)
03-17 18:19:18.394 16927 16927 F DEBUG   :       #06 pc 0000000000621fd8  /data/app/~~Ks3JY4pqes9m0gwzSWYDQw==/org.godotengine.tarnish-7ALG45aOnZwqwsvmqkQjBQ==/lib/arm64/libtarnish_rust.so (rust_begin_unwind+56)
03-17 18:19:18.394 16927 16927 F DEBUG   :       #07 pc 000000000063ce5c  /data/app/~~Ks3JY4pqes9m0gwzSWYDQw==/org.godotengine.tarnish-7ALG45aOnZwqwsvmqkQjBQ==/lib/arm64/libtarnish_rust.so (core::panicking::panic_fmt::hc62e9e70e1db8b93+44)
03-17 18:19:18.394 16927 16927 F DEBUG   :       #08 pc 000000000063d1d0  /data/app/~~Ks3JY4pqes9m0gwzSWYDQw==/org.godotengine.tarnish-7ALG45aOnZwqwsvmqkQjBQ==/lib/arm64/libtarnish_rust.so (core::panicking::assert_failed_inner::h1e933bb60f07571f+272)
03-17 18:19:18.394 16927 16927 F DEBUG   :       #09 pc 00000000005fed70  /data/app/~~Ks3JY4pqes9m0gwzSWYDQw==/org.godotengine.tarnish-7ALG45aOnZwqwsvmqkQjBQ==/lib/arm64/libtarnish_rust.so (core::panicking::assert_failed::hc8084b7fe27edcd5+52)
03-17 18:19:18.394 16927 16927 F DEBUG   :       #10 pc 00000000003198ec  /data/app/~~Ks3JY4pqes9m0gwzSWYDQw==/org.godotengine.tarnish-7ALG45aOnZwqwsvmqkQjBQ==/lib/arm64/libtarnish_rust.so (godot_core::private::handle_panic_with_print::h767f77e15a6bd188+2224)
03-17 18:19:18.394 16927 16927 F DEBUG   :       #11 pc 0000000000314d9c  /data/app/~~Ks3JY4pqes9m0gwzSWYDQw==/org.godotengine.tarnish-7ALG45aOnZwqwsvmqkQjBQ==/lib/arm64/libtarnish_rust.so (godot_core::private::handle_panic::h1762d4504f5cad13+120)
03-17 18:19:18.394 16927 16927 F DEBUG   :       #12 pc 00000000002e35a0  /data/app/~~Ks3JY4pqes9m0gwzSWYDQw==/org.godotengine.tarnish-7ALG45aOnZwqwsvmqkQjBQ==/lib/arm64/libtarnish_rust.so (godot_core::init::ffi_initialize_layer::ha2156e5340c87084+64)
03-17 18:19:18.394 16927 16927 F DEBUG   :       #13 pc 0000000003816c24  /data/app/~~Ks3JY4pqes9m0gwzSWYDQw==/org.godotengine.tarnish-7ALG45aOnZwqwsvmqkQjBQ==/lib/arm64/libgodot_android.so
03-17 18:19:18.394 16927 16927 F DEBUG   :       #14 pc 0000000003837850  /data/app/~~Ks3JY4pqes9m0gwzSWYDQw==/org.godotengine.tarnish-7ALG45aOnZwqwsvmqkQjBQ==/lib/arm64/libgodot_android.so
03-17 18:19:18.394 16927 16927 F DEBUG   :       #15 pc 0000000000e55538  /data/app/~~Ks3JY4pqes9m0gwzSWYDQw==/org.godotengine.tarnish-7ALG45aOnZwqwsvmqkQjBQ==/lib/arm64/libgodot_android.so
03-17 18:19:18.394 16927 16927 F DEBUG   :       #16 pc 0000000000e2de68  /data/app/~~Ks3JY4pqes9m0gwzSWYDQw==/org.godotengine.tarnish-7ALG45aOnZwqwsvmqkQjBQ==/lib/arm64/libgodot_android.so (Java_org_godotengine_godot_G```

jeyum2 · 2024-07-14T22:45:51Z

I have confirmed that the same panic occurs on Android with the master branch where #794 has been merged.
I have rebased and modified the code to add 'android' to the cfg flag.

This code block causes a runtime panic during initializing in Android debug builds Signed-off-by: Jan Haller <bromeon@gmail.com>

Bromeon · 2024-07-19T16:17:22Z

While I'm not a fan of not getting to the root of things and just silencing the problem, it doesn't help anyone if Android setup is just totally broken, either. I added a comment in the code, hopefully someone with more expertise can investigate this in the future.

Bromeon added bug c: tooling CI, automation, tools labels Jun 26, 2024

jeyum2 changed the title ~~Disable main_thread_id assertion for debug build~~ Disable main_thread_id assertion for Android debug build Jun 26, 2024

jeyum2 force-pushed the fix-android-panic branch from 12e423b to df4a5d3 Compare June 26, 2024 12:36

jeyum2 force-pushed the fix-android-panic branch from df4a5d3 to adb70d0 Compare June 27, 2024 00:19

Bromeon reviewed Jul 8, 2024

View reviewed changes

Bromeon added the c: android Android export target label Jul 10, 2024

jeyum2 force-pushed the fix-android-panic branch from adb70d0 to de0f6c0 Compare July 14, 2024 22:43

Disable main_thread_id assertion for Android debug build

11b0ab7

This code block causes a runtime panic during initializing in Android debug builds Signed-off-by: Jan Haller <bromeon@gmail.com>

Bromeon force-pushed the fix-android-panic branch from de0f6c0 to 11b0ab7 Compare July 19, 2024 16:16

Bromeon enabled auto-merge July 19, 2024 16:17

Bromeon added this pull request to the merge queue Jul 19, 2024

Merged via the queue into godot-rust:master with commit 3e24714 Jul 19, 2024

jeyum2 deleted the fix-android-panic branch July 20, 2024 01:31

jeyum2 restored the fix-android-panic branch July 20, 2024 01:31

jeyum2 deleted the fix-android-panic branch July 20, 2024 01:32

Uh oh!

Conversation

jeyum2 commented Jun 26, 2024

Uh oh!

Bromeon commented Jun 26, 2024

Uh oh!

jeyum2 commented Jun 26, 2024

Uh oh!

Bromeon commented Jun 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeyum2 commented Jun 26, 2024

Uh oh!

GodotRust commented Jun 27, 2024

Uh oh!

Bromeon left a comment

Choose a reason for hiding this comment

Uh oh!

Bromeon Jul 8, 2024

Choose a reason for hiding this comment

Uh oh!

scripturial commented Jul 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bromeon commented Jul 9, 2024

Uh oh!

Bromeon commented Jul 14, 2024

Uh oh!

jeyum2 commented Jul 14, 2024

Uh oh!

jeyum2 commented Jul 14, 2024

Uh oh!

Bromeon commented Jul 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Bromeon commented Jun 26, 2024 •

edited

Loading

scripturial commented Jul 9, 2024 •

edited

Loading